test(log): new unit tests #7988

velvetrevolver · 2024-09-13T13:14:48Z

Description

write unit tests for logCapture

XO-184

Checklist

Commit
- Title follows commit conventions
- Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
- If bug fix, add Introduced by
Changelog
- If visible by XOA users, add changelog entry
- Update "Packages to release" in CHANGELOG.unreleased.md
PR
- If UI changes, add screenshots
- If not finished or not tested, open as Draft

julien-f

Too many of these tests are not related to captureLogs(), if you want to keep them, move them to another commit & file.

@xen-orchestra/log/capture.test.js

julien-f · 2024-09-16T08:51:44Z

@xen-orchestra/log/capture.test.js

+    assert.equal(logsTransportDefault[0]?.message, expected1.message)
+    assert.equal(typeof logsTransportDefault[0]?.level, 'number', `level attribute should exist`)
+    assert.ok(typeof logsTransportDefault[0]?.namespace === 'string', `namespace attribute should exist`)
+    assert.ok(logsTransportDefault[0]?.time instanceof Date, `time attribute should exist`)


Do not use optional chaining when you know the entry exist.

julien-f · 2024-09-16T08:53:42Z

@xen-orchestra/log/capture.test.js

+    }
+    logger.debug(expected1.message)
+    assert.equal(logsTransportDefault[0]?.message, expected1.message)
+    assert.equal(typeof logsTransportDefault[0]?.level, 'number', `level attribute should exist`)


Do not bother adding an assert message unless it brings a new info.

In this case, I don't see how it would really help, if the test fails, we know which assert did and we fix it.

To be clear, it's absolutely not a problem that you provided a message, but you don't have to 🙂

julien-f · 2024-09-16T08:54:15Z

@xen-orchestra/log/capture.test.js

+    logger.debug(expected1.message)
+    assert.equal(logsTransportDefault[0]?.message, expected1.message)
+    assert.equal(typeof logsTransportDefault[0]?.level, 'number', `level attribute should exist`)
+    assert.ok(typeof logsTransportDefault[0]?.namespace === 'string', `namespace attribute should exist`)


Use assert.equal instead because it will show the actual value which is important to understand the issue when the assert fails.

julien-f · 2024-09-16T08:56:06Z

@xen-orchestra/log/capture.test.js

+    logsTransportFatal.length = 0
+  })
+
+  it('should log test with all its attributes', () => {


That's not what you are testing here: you are testing that logs not in captureLogs() are handled by the fallback transport.

I tested what appeared in the Usage.md, here it was the "producer" part presenting the logger.

julien-f · 2024-09-16T08:56:39Z

@xen-orchestra/log/capture.test.js

+    assert.ok(typeof logsTransportDefault[0]?.namespace === 'string', `namespace attribute should exist`)
+    assert.ok(logsTransportDefault[0]?.time instanceof Date, `time attribute should exist`)
+  })
+  it('should log test 1 and 2', () => {


I don't understand what you are testing here and how it is related to captureLogs.

I tested in order what was in the Usage.md, there it was the "producer" part presenting the logger.

@xen-orchestra/log/capture.test.js

julien-f · 2024-09-22T15:26:22Z

@xen-orchestra/log/capture.test.js

+  it('should not dedup logs', () => {
+    configure(
+      createCaptureTransport([
+        {
+          transport: transportTest,
+        },
+      ])
+    )
+
+    for (let i = 0; i < 3; i++) {
+      logger.debug('this line should be logged 3 times')
+    }
+    assert.equal(logsTransportDefault.length, 3)
+  })
+  it('should dedup logs', () => {
+    configure(
+      createCaptureTransport([
+        dedupe({
+          timeout: 50, // without a defined timeout, the test will wait for 10 minutes
+          transport: transportTest,
+        }),
+      ])
+    )
+
+    for (let i = 0; i < 3; i++) {
+      logger.debug('this line should be logged only once')
+    }
+    assert.equal(logsTransportDefault.length, 1)
+  })


Out of captureLogs() scope, you can move into its own file.

julien-f · 2024-09-22T15:28:13Z

@xen-orchestra/log/logger.test.js

+    const expected1 = {
+      level: 20,
+      namespace: 'test-logger',
+      message: 'test 1 with all its attributes',
+      time: new Date(),
+    }


Why are you creating this object? you are not using it? 🤔

The message is tested line 49 :
assert.equal(logsTransportDefault[0].message, expected1.message)

Yes but you don't need this object, you can simply create a message variable if that's all you need.

julien-f · 2024-09-22T15:29:17Z

@xen-orchestra/log/logger.test.js

+    assert.equal(typeof logsTransportDefault[0].namespace, 'string', `namespace attribute should exist`)
+    assert.equal(logsTransportDefault[0].time instanceof Date, true, 'time attribute should exist')
+  })
+  it('should log test 1 and 2', () => {


What feature does this case test that the the previous one did not?

It test the "test transport" can contain many logs, and not only one constantly overwriten. Needed by the captureLog test after when we test multiple logs.

@xen-orchestra/log/logger.test.js

Co-authored-by: Julien Fontanet <[email protected]>

velvetrevolver requested a review from julien-f September 13, 2024 13:14

velvetrevolver self-assigned this Sep 13, 2024

julien-f requested changes Sep 16, 2024

View reviewed changes

velvetrevolver requested a review from julien-f September 16, 2024 23:36

julien-f changed the title ~~write unit tests for logCapture~~ test(log): new unit tests Sep 20, 2024

julien-f reviewed Sep 22, 2024

View reviewed changes

@xen-orchestra/log/capture.test.js Outdated Show resolved Hide resolved

julien-f reviewed Sep 22, 2024

View reviewed changes

@xen-orchestra/log/logger.test.js Outdated Show resolved Hide resolved

velvetrevolver force-pushed the capture-logs-tests branch from d2497fa to e0400e7 Compare September 24, 2024 09:06

velvetrevolver requested a review from julien-f September 25, 2024 09:49

julien-f force-pushed the captureLogs branch from 6131041 to 1e051bd Compare September 25, 2024 11:44

julien-f force-pushed the capture-logs-tests branch from e0400e7 to d13fc22 Compare September 25, 2024 11:45

julien-f force-pushed the captureLogs branch from 1e051bd to ae59653 Compare October 2, 2024 12:45

julien-f force-pushed the capture-logs-tests branch from b9ef692 to defa4ae Compare October 2, 2024 12:45

Base automatically changed from captureLogs to master October 2, 2024 13:14

julien-f force-pushed the capture-logs-tests branch 2 times, most recently from 2bc6ddf to f41bcbb Compare October 8, 2024 08:57

julien-f force-pushed the capture-logs-tests branch from f41bcbb to 4197370 Compare October 10, 2024 14:37

julien-f force-pushed the capture-logs-tests branch 4 times, most recently from 162703f to 79d0e56 Compare October 23, 2024 10:10

stephane-m-dev and others added 6 commits October 28, 2024 17:43

write unit tests for logCapture

61815d8

Changes after code review

342e009

sort imports

35737b9

Update logger.test.js

5c202ce

Update @xen-orchestra/log/capture.test.js

558138f

Co-authored-by: Julien Fontanet <[email protected]>

Changes after code review

463da5b

velvetrevolver and others added 4 commits October 28, 2024 17:43

Update @xen-orchestra/log/logger.test.js

cd1de0b

Co-authored-by: Julien Fontanet <[email protected]>

Update @xen-orchestra/log/logger.test.js

fead683

Co-authored-by: Julien Fontanet <[email protected]>

Update @xen-orchestra/log/capture.test.js

50d62c6

Co-authored-by: Julien Fontanet <[email protected]>

wip

8b41248

julien-f force-pushed the capture-logs-tests branch from 79d0e56 to 8b41248 Compare October 28, 2024 16:43

velvetrevolver assigned stephane-m-dev and unassigned velvetrevolver Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(log): new unit tests #7988

test(log): new unit tests #7988

velvetrevolver commented Sep 13, 2024

julien-f left a comment

julien-f Sep 16, 2024

julien-f Sep 16, 2024

julien-f Sep 16, 2024

julien-f Sep 16, 2024

velvetrevolver Sep 16, 2024

julien-f Sep 16, 2024

velvetrevolver Sep 16, 2024

julien-f Sep 22, 2024

julien-f Sep 22, 2024

velvetrevolver Sep 24, 2024 •

edited

Loading

julien-f Sep 24, 2024

julien-f Sep 22, 2024

velvetrevolver Sep 24, 2024

test(log): new unit tests #7988

Are you sure you want to change the base?

test(log): new unit tests #7988

Conversation

velvetrevolver commented Sep 13, 2024

Description

Checklist

julien-f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

velvetrevolver Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

velvetrevolver Sep 24, 2024 •

edited

Loading